Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: allow services to indicate their dependencies #1762

Closed
wants to merge 2 commits into from

Conversation

achingbrain
Copy link
Member

Some services depend on others, for example circuit relay v2 and kad-dht don't really work without identify so we need a way to prompt the user to configure these dependencies.

The change is to allow extending the components a service requires with a service map of the dependencies.

The components type in the Libp2pInit type is extended by the ServiceMap generic type so when the user configures a node without the required dependencies, TypeScript will fail to compile as the components being passed to the factory function don't have type overlap with the required components.

Some services depend on others, for example circuit relay v2 and
kad-dht don't really work without identify so we need a way to
prompt the user to configure these dependencies.

The change is to allow extending the components a service requires
with a service map of the dependencies.

The components type in the `Libp2pInit` type is extended by the
`ServiceMap` generic type so when the user configures a node without
the required dependencies, TypeScript will fail to compile as the
components being passed to the factory function don't have type
overlap with the required components.
@@ -17,6 +18,7 @@ describe('circuit-relay discovery', () => {

beforeEach(async () => {
// create relay first so it has time to advertise itself via content routing
// @ts-expect-error TODO: typescript cannot infer the service map type
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why TypeScript can't infer this, it seems to work elsewhere.

@joeltg
Copy link
Contributor

joeltg commented May 18, 2023

Super cool!

This does feel like it gets close to the line where TypeScript stops being fun anymore... once you start playing the generics game it's hard to stop. Personally I love going crazy with types but it can start to slow people down; it's up to you to weigh those tradeoffs.

Just a thought: if the idea is that "we have some components that get passed around, which always include certain built-in components, ", then would changing everything to take a Components extends BuiltInComponents parameter make more sense than passing around the "service map" just to pass it back into Components<ServiceMap> = BuiltInComponents & ServiceMap? Might be a lot of refactoring but it could eliminate the extra "service map" type concept.

Edit: I guess the other question hanging over this is that the services refactor kinda suggests that people can name their services anything they want now, but in reality dependent services still rely on specific names. Maybe make sure this is communicated in the docs somewhere?

@achingbrain achingbrain changed the title fix: allow services to indicate their depdencies fix: allow services to indicate their dependencies May 18, 2023
@maschad
Copy link
Member

maschad commented May 18, 2023

then would changing everything to take a Components extends BuiltInComponents parameter make more sense than passing around the "service map" just to pass it back into Components = BuiltInComponents & ServiceMap? Might be a lot of refactoring but it could eliminate the extra "service map" type concept.

That's a good point @joeltg , the original impetus to even have these optional services was to reduce Libp2p surface area in consumers and also to improve extendability through custom modules. I think at the time we went with the ServiceMap because we were unsure how could have an interface that could facilitate them being accessed from a services key.

I think having the DefaultComponents extend the BuiltInComponents and also having that configuration validated would solve this issue without a large refactor.

Edit: I guess the other question hanging over this is that the services refactor kinda suggests that people can name their services anything they want now, but in reality dependent services still rely on specific names. Maybe make sure this is communicated in the docs somewhere?

Whilst we do now support custom modules I agree the docs should reflect those constraints, perhaps we could have a section in the configuration doc where we indicate that some services require others e.g. setup with relay requires identify ? I opened an issue for further discussion.

@achingbrain
Copy link
Member Author

Discussion: #2135

@achingbrain
Copy link
Member Author

I'm going to close this in favour of the solution proposed in #2135

@achingbrain achingbrain deleted the fix/allow-services-to-express-dependencies branch November 27, 2023 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants